[SPARK-25408] Move to more ideomatic Java8 by Fokko · Pull Request #22637 · apache/spark

Fokko · 2018-10-05T04:59:56Z

While working on another PR, I noticed that there is quite some legacy Java in there that can be beautified. For example the use of features from Java8, such as:

Collection libraries
Try-with-resource blocks

No logic has been changed. I think it is important to have a solid codebase with examples that will inspire next PR's to follow up on the best practices.

What are your thoughts on this?

This makes code easier to read, and using try-with-resource makes is less likely to forget to close something.

What changes were proposed in this pull request?

No changes in the logic of Spark, but more in the aesthetics of the code.

How was this patch tested?

Using the existing unit tests. Since no logic is changed, the existing unit tests should pass.

Please review http://spark.apache.org/contributing.html before opening a pull request.

HyukjinKwon · 2018-10-05T05:03:35Z

ok to test

cloud-fan · 2018-10-05T05:03:41Z

ok to test

HyukjinKwon · 2018-10-05T05:03:54Z

mind filling PR description please?

srowen · 2018-10-05T05:05:51Z

...-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleIntegrationSuite.java

Nit: move this onto the previous line

srowen · 2018-10-05T05:08:06Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/CLIService.java

I wouldn't bother with this. This is a copy of some Hive code anyway

this should still be reverted

srowen · 2018-10-05T05:08:17Z

Jenkins test this please

SparkQA · 2018-10-05T05:31:05Z

Test build #96969 has finished for PR 22637 at commit 5d3d0c7.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-05T05:34:09Z

Test build #96970 has finished for PR 22637 at commit 0155b7a.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

Fokko · 2018-10-05T05:45:12Z

Valid points. Personally I'm a fan of explicit final, instead of implicit. But that's a matter of taste :-)

SparkQA · 2018-10-05T06:03:55Z

Test build #96972 has finished for PR 22637 at commit 14d9d1f.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

SparkQA · 2018-10-05T06:34:42Z

Test build #96974 has finished for PR 22637 at commit 004ee44.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

SparkQA · 2018-10-05T07:05:01Z

Test build #96967 has finished for PR 22637 at commit 5d3d0c7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-10-05T07:05:02Z

Test build #96975 has finished for PR 22637 at commit 58de6a3.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

cloud-fan · 2018-10-05T07:37:36Z

retest this please

SparkQA · 2018-10-05T12:32:26Z

Test build #96979 has finished for PR 22637 at commit 58de6a3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

dongjoon-hyun · 2018-10-05T18:31:19Z

...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java

Indentation?

dongjoon-hyun · 2018-10-05T18:33:28Z

...ork-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleSecuritySuite.java

Indentation? It seems to follow 4-space indentation for Java style, but it seems to be too outstanding in this file. For the rest of the patch, could you adjust a little bit more to be consistent in a single file?

vanzin · 2018-10-05T18:40:19Z

While we're at it, it would be great to fix the typo in the PR title.

Fokko · 2018-10-06T05:04:59Z

Thanks @dongjoon-hyun. I've fixed the indentation issues.

SparkQA · 2018-10-06T05:15:53Z

Test build #97031 has finished for PR 22637 at commit db061b8.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

srowen · 2018-10-06T16:27:44Z

...lyst/src/test/java/org/apache/spark/sql/catalyst/expressions/RowBasedKeyValueBatchSuite.java

There's a compile error here. But note you can have multiple Closeables declared in one try block anyway, so these can be merged.

My mistake. thanks!

srowen · 2018-10-06T17:21:32Z

...lyst/src/test/java/org/apache/spark/sql/catalyst/expressions/RowBasedKeyValueBatchSuite.java

It's hard to know the best way to wrap these things, but here you have the continuation line indented less than the one that starts the statement. I think we can't do that. I'd indent both continuations 2 spaces from their preceding line ideally. Here and elsewhere where the try statement has to wrap.

srowen · 2018-10-06T17:22:12Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/CLIService.java

this should still be reverted

SparkQA · 2018-10-06T21:29:13Z

Test build #97063 has finished for PR 22637 at commit 4c084f9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

Use features from Java8 such as: - Try-with-resource blocks

SparkQA · 2018-10-07T04:08:09Z

Test build #97072 has finished for PR 22637 at commit 9c9f065.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public abstract class RowBasedKeyValueBatch extends MemoryConsumer implements Closeable

srowen · 2018-10-08T14:58:21Z

OK, trying this again. Tests have definitely run this time and we've had another good pass at small review changes.

While working on another PR, I noticed that there is quite some legacy Java in there that can be beautified. For example the use of features from Java8, such as: - Collection libraries - Try-with-resource blocks No logic has been changed. I think it is important to have a solid codebase with examples that will inspire next PR's to follow up on the best practices. What are your thoughts on this? This makes code easier to read, and using try-with-resource makes is less likely to forget to close something. ## What changes were proposed in this pull request? No changes in the logic of Spark, but more in the aesthetics of the code. ## How was this patch tested? Using the existing unit tests. Since no logic is changed, the existing unit tests should pass. Please review http://spark.apache.org/contributing.html before opening a pull request. Closes apache#22637 from Fokko/SPARK-25408. Authored-by: Fokko Driesprong <fokkodriesprong@godatadriven.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

Fokko mentioned this pull request Oct 5, 2018

[SPARK-25408] Move to mode ideomatic Java8 #22399

Closed

Fokko changed the title ~~Spark 25408~~ [SPARK-25408] Move to mode ideomatic Java8 Oct 5, 2018

Fokko force-pushed the SPARK-25408 branch from 99fd3a5 to 5d3d0c7 Compare October 5, 2018 05:04

srowen reviewed Oct 5, 2018

View reviewed changes

Fokko force-pushed the SPARK-25408 branch from 5d3d0c7 to 0155b7a Compare October 5, 2018 05:13

Fokko force-pushed the SPARK-25408 branch from 0155b7a to 14d9d1f Compare October 5, 2018 05:44

Fokko force-pushed the SPARK-25408 branch from 14d9d1f to 004ee44 Compare October 5, 2018 06:14

Fokko force-pushed the SPARK-25408 branch from 004ee44 to 58de6a3 Compare October 5, 2018 06:56

dongjoon-hyun reviewed Oct 5, 2018

View reviewed changes

...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated

Copy link

Member

dongjoon-hyun Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation?

dongjoon-hyun reviewed Oct 5, 2018

View reviewed changes

Fokko changed the title ~~[SPARK-25408] Move to mode ideomatic Java8~~ [SPARK-25408] Move to more ideomatic Java8 Oct 6, 2018

Fokko force-pushed the SPARK-25408 branch from 58de6a3 to db061b8 Compare October 6, 2018 05:04

srowen reviewed Oct 6, 2018

View reviewed changes

Fokko force-pushed the SPARK-25408 branch from db061b8 to 4c084f9 Compare October 6, 2018 17:04

srowen requested changes Oct 6, 2018

View reviewed changes

[SPARK-25408] Move to mode ideomatic Java8

9c9f065

Use features from Java8 such as: - Try-with-resource blocks

Fokko force-pushed the SPARK-25408 branch from 4c084f9 to 9c9f065 Compare October 6, 2018 23:38

srowen approved these changes Oct 7, 2018

View reviewed changes

asfgit closed this in 1a28625 Oct 8, 2018

Fokko deleted the SPARK-25408 branch July 14, 2020 19:20

Conversation

Fokko commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Oct 5, 2018

Uh oh!

cloud-fan commented Oct 5, 2018

Uh oh!

HyukjinKwon commented Oct 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

Fokko commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

cloud-fan commented Oct 5, 2018

Uh oh!

SparkQA commented Oct 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Oct 5, 2018

Uh oh!

Fokko commented Oct 6, 2018

Uh oh!

SparkQA commented Oct 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 6, 2018

Uh oh!

SparkQA commented Oct 7, 2018

Uh oh!

srowen commented Oct 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fokko commented Oct 5, 2018 •

edited

Loading

dongjoon-hyun Oct 5, 2018 •

edited

Loading